no-issue: disable tests conditionally when binary is not present by ricardozanini · Pull Request #1191 · serverlessworkflow/sdk-java

ricardozanini · 2026-02-27T19:34:21Z

Many thanks for submitting your Pull Request ❤️!

What this PR does / why we need it:
Sometimes, a local machine environment might not have python or protoc installed to run "unit" tests. In my opinion, these tests should be ITs and be on a specific profile.

However, if we want to run as unit tests and they depend on the environment where they're running, we disable the test if needed.

Special notes for reviewers:
mvn clean install now runs smoothly if python or grpc are not available.

Additional information (if needed):

Signed-off-by: Ricardo Zanini <ricardozanini@gmail.com>

Copilot

Pull request overview

Adds a JUnit 5 annotation/extension to conditionally disable tests when required external binaries (e.g., protoc, python) are not available on the executing machine, improving mvn clean install reliability across environments.

Changes:

Introduce @DisabledIfBinaryUnavailable plus BinaryCheckCondition JUnit 5 ExecutionCondition.
Apply the new annotation to gRPC tests requiring protoc.
Apply the new annotation to the parameterized custom function test requiring python.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
impl/test/src/test/java/io/serverlessworkflow/impl/test/junit/DisabledIfBinaryUnavailable.java	New annotation to register the binary-check execution condition.
impl/test/src/test/java/io/serverlessworkflow/impl/test/junit/BinaryCheckCondition.java	New JUnit 5 `ExecutionCondition` that runs a command to determine binary availability.
impl/test/src/test/java/io/serverlessworkflow/impl/test/grpc/GrpcUnaryTest.java	Disable test class when `protoc` is unavailable.
impl/test/src/test/java/io/serverlessworkflow/impl/test/grpc/GrpcUnaryArgsExprTest.java	Disable test class when `protoc` is unavailable.
impl/test/src/test/java/io/serverlessworkflow/impl/test/grpc/GrpcServerStreamingTest.java	Disable test class when `protoc` is unavailable.
impl/test/src/test/java/io/serverlessworkflow/impl/test/grpc/GrpcClientStreamingTest.java	Disable test class when `protoc` is unavailable.
impl/test/src/test/java/io/serverlessworkflow/impl/test/grpc/GrpcBiDirectionalStreamingTest.java	Disable test class when `protoc` is unavailable.
impl/test/src/test/java/io/serverlessworkflow/impl/test/CustomFunctionTest.java	Disable the python-dependent parameterized test when `python` is unavailable.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

impl/test/src/test/java/io/serverlessworkflow/impl/test/junit/BinaryCheckCondition.java

Signed-off-by: Ricardo Zanini <ricardozanini@gmail.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

impl/test/src/test/java/io/serverlessworkflow/impl/test/grpc/GrpcUnaryTest.java

impl/test/src/test/java/io/serverlessworkflow/impl/test/grpc/GrpcUnaryArgsExprTest.java

impl/test/src/test/java/io/serverlessworkflow/impl/test/grpc/GrpcServerStreamingTest.java

impl/test/src/test/java/io/serverlessworkflow/impl/test/grpc/GrpcClientStreamingTest.java

.../test/src/test/java/io/serverlessworkflow/impl/test/grpc/GrpcBiDirectionalStreamingTest.java

impl/test/src/test/java/io/serverlessworkflow/impl/test/junit/BinaryCheckCondition.java

Signed-off-by: Ricardo Zanini <ricardozanini@gmail.com>

fjtirado · 2026-03-02T11:43:57Z

impl/test/src/test/java/io/serverlessworkflow/impl/test/junit/BinaryCheckUtil.java

+          try {
+            Process process =
+                new ProcessBuilder(command)
+                    .redirectErrorStream(true)
+                    .redirectOutput(ProcessBuilder.Redirect.DISCARD)
+                    .start();
+            boolean finished = process.waitFor(2, TimeUnit.SECONDS);
+            if (finished) {
+              return process.exitValue() == 0;
+            }
+            process.destroyForcibly();
+            return false;
+          } catch (IOException | InterruptedException e) {
+            if (e instanceof InterruptedException) {
+              Thread.currentThread().interrupt();
+            }
+            return false;
+          }


Suggested change

try {

Process process =

new ProcessBuilder(command)

.redirectErrorStream(true)

.redirectOutput(ProcessBuilder.Redirect.DISCARD)

.start();

boolean finished = process.waitFor(2, TimeUnit.SECONDS);

if (finished) {

return process.exitValue() == 0;

}

process.destroyForcibly();

return false;

} catch (IOException | InterruptedException e) {

if (e instanceof InterruptedException) {

Thread.currentThread().interrupt();

}

return false;

}

ProcessBuilder processBuilder =

new ProcessBuilder(command)

.redirectErrorStream(true)

.redirectOutput(ProcessBuilder.Redirect.DISCARD);

Process process = null;

try {

process = process.start();

return process.waitFor(2, TimeUnit.SECONDS) && process.exitValue() == 0;

} catch (IOException e) {

// return default value

}

catch (InterruptedException e) {

Thread.currentThread().interrupt();

}

finally {

if (process != null) {

process.destroyForcibly();

}

}

return false;

You will need to format this, but the idea is always destroy the process (with current implementation if it is not destroyed if something goes wrong), do not wrap process builder within the finally block and return false except when there is a process to query about.
And also avoid and unneeded instanceof by using two catch blocks.

Another alternative can be

Suggested change

try {

Process process =

new ProcessBuilder(command)

.redirectErrorStream(true)

.redirectOutput(ProcessBuilder.Redirect.DISCARD)

.start();

boolean finished = process.waitFor(2, TimeUnit.SECONDS);

if (finished) {

return process.exitValue() == 0;

}

process.destroyForcibly();

return false;

} catch (IOException | InterruptedException e) {

if (e instanceof InterruptedException) {

Thread.currentThread().interrupt();

}

return false;

}

Process process;

try {

process = new ProcessBuilder(command)

.redirectErrorStream(true)

.redirectOutput(ProcessBuilder.Redirect.DISCARD).start();

} catch (IOException e) {

return false;

}

try {

return process.waitFor(2, TimeUnit.SECONDS) && process.exitValue() == 0;

}

catch (InterruptedException e) {

Thread.currentThread().interrupt();

return false;

}

finally {

process.destroyForcibly();

}

fjtirado · 2026-03-02T11:51:48Z

impl/test/src/test/java/io/serverlessworkflow/impl/test/junit/ProtocCheckCondition.java

+            annotation -> {
+              if (isProtocAvailable()) {
+                return ConditionEvaluationResult.enabled(
+                    "Protoc is available for the current platform.");
+              } else {
+                return ConditionEvaluationResult.disabled(
+                    "Test disabled: Protoc not currently available not found.");
+              }
+            })


Suggested change

annotation -> {

if (isProtocAvailable()) {

return ConditionEvaluationResult.enabled(

"Protoc is available for the current platform.");

} else {

return ConditionEvaluationResult.disabled(

"Test disabled: Protoc not currently available not found.");

}

})

annotation ->

isProtocAvailable() ? ConditionEvaluationResult.enabled(

"Protoc is available for the current platform.") : ConditionEvaluationResult.disabled(

"Test disabled: Protoc not currently available not found."))

fjtirado · 2026-03-02T12:33:26Z

impl/test/src/test/java/io/serverlessworkflow/impl/test/junit/ProtocCheckCondition.java

+
+public class ProtocCheckCondition implements ExecutionCondition {
+
+  private static final AtomicReference<Boolean> protocAvailable = new AtomicReference<>(null);


Suggested change

private static final AtomicReference<Boolean> protocAvailable = new AtomicReference<>(null);

private static boolean protocAvailable;

static {

try {

protocAvailable = Protoc.runProtoc(new String[] {"--version"}) == 0;

} catch (Exception e) {

// ignore

}

if (!protocAvailable) {

protoCAvailable= isBinaryAvailable("protoc", "--version");

}

}

fjtirado · 2026-03-02T12:34:11Z

impl/test/src/test/java/io/serverlessworkflow/impl/test/junit/ProtocCheckCondition.java

+    Boolean cached = protocAvailable.get();
+    if (cached != null) {
+      return cached;
+    }
+
+    boolean found = false;
+    try {
+      if (Protoc.runProtoc(new String[] {"--version"}) == 0) found = true;
+    } catch (Exception e) {
+      // ignore
+    }
+
+    if (!found) found = isBinaryAvailable("protoc", "--version");
+
+    protocAvailable.compareAndSet(null, found);
+
+    return protocAvailable.get();


Suggested change

Boolean cached = protocAvailable.get();

if (cached != null) {

return cached;

}

boolean found = false;

try {

if (Protoc.runProtoc(new String[] {"--version"}) == 0) found = true;

} catch (Exception e) {

// ignore

}

if (!found) found = isBinaryAvailable("protoc", "--version");

protocAvailable.compareAndSet(null, found);

return protocAvailable.get();

return protocAvailable;

fjtirado · 2026-03-02T12:35:41Z

impl/test/src/test/java/io/serverlessworkflow/impl/test/junit/PythonCheckCondition.java

+            annotation -> {
+              if (isBinaryAvailable("python", "--version")) {
+                return ConditionEvaluationResult.enabled("Python is available.");
+              } else {
+                return ConditionEvaluationResult.disabled("Test disabled: Python not found.");
+              }
+            })


Suggested change

annotation -> {

if (isBinaryAvailable("python", "--version")) {

return ConditionEvaluationResult.enabled("Python is available.");

} else {

return ConditionEvaluationResult.disabled("Test disabled: Python not found.");

}

})

annotation -> {

isBinaryAvailable("python", "--version") ?

ConditionEvaluationResult.enabled("Python is available.") :

ConditionEvaluationResult.disabled("Test disabled: Python not found."))

fjtirado

Good idea.
I have some non trivial comments regarding implementation, though

fjtirado · 2026-03-02T13:02:32Z

@ricardozanini I just realized that, if protoc binary is not in classpath, it will be extracted from the jar dependency, so for grpc I do not think the check is needed (what Im trying to say if that the test will work in systems, like mine, where protoc is not installed, do not need to disable it, because isProtoCAvailable will always be true once the protoc dependency is there)
For Python, your rationale is good, so lets remove the protoc part and keep the python one of this PR

See https://github.com/os72/protoc-jar/blob/master/src/main/java/com/github/os72/protocjar/Protoc.java#L189-L230. In practise, something will be really screwed up if isProtoCAvailable were false, and probably in that case we should fail CI rather than go on.

ricardozanini added 2 commits February 27, 2026 14:29

no-issue: disable tests conditionally when binary is not present

b27bac9

Signed-off-by: Ricardo Zanini <ricardozanini@gmail.com>

Remove utils

59bb9a0

Signed-off-by: Ricardo Zanini <ricardozanini@gmail.com>

ricardozanini requested a review from fjtirado as a code owner February 27, 2026 19:34

ricardozanini requested review from Copilot and mcruzdev February 27, 2026 19:34

Copilot started reviewing on behalf of ricardozanini February 27, 2026 19:34 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

Incorporating copilot comments

6822659

Signed-off-by: Ricardo Zanini <ricardozanini@gmail.com>

ricardozanini requested a review from Copilot February 27, 2026 21:26

Copilot started reviewing on behalf of ricardozanini February 27, 2026 21:26 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

Check protoc via jar first

9706f7f

Signed-off-by: Ricardo Zanini <ricardozanini@gmail.com>

fjtirado reviewed Mar 2, 2026

View reviewed changes


		public class ProtocCheckCondition implements ExecutionCondition {

		private static final AtomicReference<Boolean> protocAvailable = new AtomicReference<>(null);

-  private static final AtomicReference<Boolean> protocAvailable = new AtomicReference<>(null);
+  private static  boolean protocAvailable;
+  static {
+    try {
+      protocAvailable = Protoc.runProtoc(new String[] {"--version"}) == 0;
+    } catch (Exception e) {
+      // ignore
+    }
+    if (!protocAvailable) {
+        protoCAvailable= isBinaryAvailable("protoc", "--version");
+     }
+  }

Conversation

ricardozanini commented Feb 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjtirado Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

fjtirado Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

fjtirado Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

fjtirado Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

fjtirado Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjtirado Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

fjtirado Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

fjtirado left a comment

Choose a reason for hiding this comment

Uh oh!

fjtirado commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fjtirado Mar 2, 2026 •

edited

Loading

fjtirado commented Mar 2, 2026 •

edited

Loading